Skip to content

feat: add helm.sh/resource-policy: keep to all chart resources#45

Merged
ian-flores merged 2 commits intomainfrom
add-resource-policy-keep-annotations
Jan 21, 2026
Merged

feat: add helm.sh/resource-policy: keep to all chart resources#45
ian-flores merged 2 commits intomainfrom
add-resource-policy-keep-annotations

Conversation

@ian-flores
Copy link
Contributor

Summary

Adds helm.sh/resource-policy: keep annotation to all Kubernetes resources created by the Helm chart.

Why

This enables safe migration from direct Pulumi Helm Release management to helm-controller (HelmChart CR) management. When Pulumi removes its Release resource, helm uninstall runs but resources with this annotation are preserved. The new HelmChart CR then adopts the existing resources via helm upgrade.

Resources Protected

  • Deployment (manager)
  • ServiceAccount
  • ClusterRoles (5), Roles (3)
  • ClusterRoleBindings (3), RoleBindings (3)
  • Service (metrics)
  • ServiceMonitor
  • Certificates (2), Issuer

Note: CRDs already have configurable protection via crd.keep: true (default).

Related

  • Plan: thoughts/shared/plans/2026-01-20-team-operator-helm-controller-migration.md
  • Phase 1 of helm-controller migration

Adds resource protection annotation to all Kubernetes resources created
by the chart. This prevents resources from being deleted during helm
uninstall, enabling safe migration to helm-controller management.

Resources protected:
- Deployment, ServiceAccount
- ClusterRoles, Roles, ClusterRoleBindings, RoleBindings
- Service, ServiceMonitor
- Certificates, Issuer
@claude
Copy link

claude bot commented Jan 20, 2026

Claude finished @ian-flores's task —— View job


Reviewing PR

  • Read review guidelines
  • Analyze changed files
  • Create pending review
  • Add inline comments (submitted in review body - inline comments require additional permissions)
  • Submit review

Review submitted with observations on the certificate template behavior change and a suggestion for configurability.

Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Overall the changes are straightforward and consistent across all 24 templates. The annotation placement follows existing patterns.

Observations:

  1. Certificate templates behavior change: The certificates (serving-cert, metrics-certs) previously used the conditional {{ if .Values.crd.keep }} pattern to match CRD behavior. This PR changes them to unconditional keep. If this is intentional, consider updating the PR description since it mentions "CRDs already have configurable protection via crd.keep: true" but certificates no longer follow this same configurable pattern.

  2. Configurability consideration: All non-CRD resources now unconditionally get the keep annotation. For flexibility after the migration is complete, consider adding a configurable value (e.g., resourcePolicy.keep: true) to mirror the crd.keep pattern. This is optional if unconditional is the desired long-term behavior.

  3. Service Account template (dist/chart/templates/rbac/service_account.yaml): The refactoring correctly ensures annotations: block is always present with helm.sh/resource-policy: keep, while preserving user-configurable annotations. Good approach.

Helm Chart Checklist:

  • Templates structure looks correct
  • Annotation consistently applied across all resources
  • RBAC templates unchanged except for annotation (no permission changes)

LGTM with minor suggestions above. The changes serve their stated purpose for the helm-controller migration.

Adds resourcePolicy.keep value (default: true) to control the
helm.sh/resource-policy: keep annotation on non-CRD resources.

Independent from crd.keep which controls CRDs only.
@ian-flores ian-flores marked this pull request as ready for review January 21, 2026 15:16
@ian-flores ian-flores merged commit ed24c9a into main Jan 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants